issue #117, Resolved cppcheck warning #118
Conversation
You get the warning: The buffer 'TaskName' may not be null-terminated after the call to strncpy(). |
|
Actually since it's just copying a constant, I'd vote to just |
|
yes. Updated. |
fsw/pc-rtems/src/cfe_psp_exception.c
Outdated
| @@ -135,7 +135,7 @@ void CFE_PSP_ExceptionHook (int task_id, int vector, int32 *pEsf ) | |||
| /* | |||
There was a problem hiding this comment.
This code is merely a placeholder. The exception processing in PC-RTEMS is not really implemented, and clearly bits and pieces were at one point copied from the vxworks psp.
My suggestion is to remove the entire block from lines 133-150, and just set CFE_PSP_ExceptionReasonString to something like UNIMPLEMENTED.
Note that the test in the if statement on line 140 is totally bogus because TaskName is a local stack array, it can never be NULL.
fsw/pc-linux/src/cfe_psp_memory.c
Outdated
| @@ -677,16 +677,31 @@ int32 CFE_PSP_InitProcessorReservedMemory( uint32 RestartType ) | |||
| if ( RestartType == CFE_PSP_RST_TYPE_PROCESSOR ) | |||
There was a problem hiding this comment.
CCB may want these to be OS_DEBUG so it has zero impact on current behavior unless debug is enabled (assuming this passes cppcheck).
There was a problem hiding this comment.
Adding #ifdef OS_DEBUG_PRINTF ... will still cause warning.
There was a problem hiding this comment.
Great point, didn't realize OS_DEBUG was a no-op if OS_DEBUG_PRINTF is not defined. How about:
if (result != whatever_the_success_code_is)
OS_printf("xyz_function didn't return success (%d)\n", return_code);
There was a problem hiding this comment.
I have several complaints about this code - we should clean it up properly rather than just squelching the cppcheck warnings about it. After all, the point of running cppcheck is to actually improve the code, not just to keep the tool quiet.
Two issues:
- the calls to the sub-functions are the same, in the same order, just with a different (hard-coded!) parameter, which should be equal to the
RestartTypevalue to begin with. So can we just call e.g.
CFE_PSP_InitCDS(RestartType)
and so on, and get rid of the if statement and else branch completely?
- Failure of any of these memory initialization routines will almost certainly cause CFE to fail later on. If any failure occurs, it should not just print it, but stop the process and return the value to the caller. Also, the caller should check the value and do a
CFE_PSP_Panic()if it fails. Currently it ignores it.
fsw/pc-linux/src/cfe_psp_memory.c
Outdated
| @@ -677,16 +677,31 @@ int32 CFE_PSP_InitProcessorReservedMemory( uint32 RestartType ) | |||
| if ( RestartType == CFE_PSP_RST_TYPE_PROCESSOR ) | |||
There was a problem hiding this comment.
I have several complaints about this code - we should clean it up properly rather than just squelching the cppcheck warnings about it. After all, the point of running cppcheck is to actually improve the code, not just to keep the tool quiet.
Two issues:
- the calls to the sub-functions are the same, in the same order, just with a different (hard-coded!) parameter, which should be equal to the
RestartTypevalue to begin with. So can we just call e.g.
CFE_PSP_InitCDS(RestartType)
and so on, and get rid of the if statement and else branch completely?
- Failure of any of these memory initialization routines will almost certainly cause CFE to fail later on. If any failure occurs, it should not just print it, but stop the process and return the value to the caller. Also, the caller should check the value and do a
CFE_PSP_Panic()if it fails. Currently it ignores it.
fsw/pc-rtems/src/cfe_psp_exception.c
Outdated
| @@ -135,7 +135,7 @@ void CFE_PSP_ExceptionHook (int task_id, int vector, int32 *pEsf ) | |||
| /* | |||
There was a problem hiding this comment.
This code is merely a placeholder. The exception processing in PC-RTEMS is not really implemented, and clearly bits and pieces were at one point copied from the vxworks psp.
My suggestion is to remove the entire block from lines 133-150, and just set CFE_PSP_ExceptionReasonString to something like UNIMPLEMENTED.
Note that the test in the if statement on line 140 is totally bogus because TaskName is a local stack array, it can never be NULL.
|
Updated. |
|
Updated. |
|
CCB 20191218 - Reviewed and approved for IC |
Describe the contribution
Resolve PSP cpp check warning
Testing performed
Steps taken to test the contribution:
cppcheck --force --inline-suppr --std=c99 --language=c --error-exitcode=1 --enable=warning,performance,portability,style --inconclusive psp/fsw 2>psp.txt
Verify warning is gone
make prep
make
make install
verify cfs still runs.
System(s) tested on:
Contributor Info
Anh Van, NASA Goddard